Support multiple external IPs in blueprint zone type by bnaecker · Pull Request #11236 · oxidecomputer/omicron

bnaecker · 2026-09-03T21:42:59Z

Add support for multiple EIPs to the blueprint zone types for Nexus, External DNS, and Boundary NTP zones.
Add some newtypes and wrappers to support lists of these or up to 2 of them for the SNAT case of Boundary NTP.
Add a test that the full blueprint with multiple addresses round-trips through the database.
Planner still emits exactly one address in all these cases, this is only the structural change to support multiple addresses.
Update lockstep OpenAPI docs

- Add support for multiple EIPs to the blueprint zone types for Nexus, External DNS, and Boundary NTP zones. - Add some newtypes and wrappers to support lists of these or up to 2 of them for the SNAT case of Boundary NTP. - Add a test that the full blueprint with multiple addresses round-trips through the database. - Planner still emits exactly one address in all these cases, this is only the structural change to support multiple addresses. - Update lockstep OpenAPI docs - Closes #9288

jgallagher · 2026-09-10T14:37:13Z

+                    .map(|kind| ServiceZoneNatEntry {
+                        zone_id: zone_config.id,
+                        sled_underlay_ip: *get_sled_address(sled_subnet).ip(),
+                        nic_mac,
+                        vni,
+                        kind,
+                    })
+                    .collect::<Vec<_>>();


Tiny nit - I don't think we need to allocate an intermediate Vec here; we can return the iterator directly

Suggested change

.map(|kind| ServiceZoneNatEntry {

zone_id: zone_config.id,

sled_underlay_ip: *get_sled_address(sled_subnet).ip(),

nic_mac,

vni,

kind,

})

.collect::<Vec<_>>();

.map(move |kind| ServiceZoneNatEntry {

zone_id: zone_config.id,

sled_underlay_ip: *get_sled_address(sled_subnet).ip(),

nic_mac,

vni,

kind,

});

jgallagher · 2026-09-10T14:43:26Z

-                ),
+                dns_addresses: zone
+                    .dns_addresses
+                    .into_external_dns_addrs_or_panic(),


Hopefully this is just a naming nit (haven't reviewed the implementation of this method yet), but I don't think *_or_panic() should be a thing. Either:

this conversion can fail, in which case it should return a result

this conversion can't fail, in which case it shouldn't need an _or_panic() suffix

It's entirely possible the method contains assertions or unwraps that we know can't fail, but that'd fall into the second category, I think - it'd be an implementation detail that doesn't need to worry the caller.

This is a good example of the allergy to From impls I mentioned in the sibling comment :)

So the only reason this exists is because we convert from BlueprintZoneType to OmicronZoneType here, which has this:

omicron/nexus/types/src/deployment/zone_type.rs

Lines 300 to 302 in ebebdfe

dns_addresses: ExternalDnsAddrs::from_single(

zone.dns_address.addr,

),

What we're really doing here is converting between two types that should have the same invariants, but are still two types that could drift. I have the proptest in here to catch that, but I was hoping this would make it more clear that there's a technical possibility of failure.

In any case, I think this is the second case you mentioned, so a From impl or the named method without _or_panic() is better.

jgallagher · 2026-09-10T14:45:34Z

 /// IP.
+//
+// NOTE: It's important that we continue to derive Ord and Eq. They're used in
+// those trait implementations for the newtype `OmicronZoneExternalFloatingIps`.


Really small nit but I'm not sure this is a useful note - if someone removes these derives, won't they get a compilation error from where they're used?

Yeah you might be right. While implementing this, I definitely confused myself into a corner trying to get Ord and also IdOrdItem to match, but that's not required. Anyway, I'll remove this, thanks.

jgallagher · 2026-09-10T14:46:12Z

+//
+// NOTE: It's important that we continue to derive Ord and Eq. They're used in
+// those trait implementations for the newtype
+// `OmicronZoneExternalFloatingAddrs`.


(same nit as above)

jgallagher · 2026-09-10T14:48:22Z

 ///
 /// ## Implementation notes
 ///
 /// `OmicronZoneNetworkResources` consists of two 1:1:1 "trijective" maps:


Is this type still meaningful / correct? IIUC it only allows one external IP per zone ID

jgallagher · 2026-09-10T18:42:52Z

    pub fn external_networking(
        &self,
-    ) -> Option<(OmicronZoneExternalIp, &NetworkInterface)> {
+    ) -> Option<(Vec<OmicronZoneExternalIp>, &NetworkInterface)> {


Returning a documented-as-nonempty Vec doesn't feel great, especially since a bunch of the callers discard it and only care about the NIC. I guess changing it to return an impl Iterator would avoid the unnecessary allocation but wouldn't fix the "I have to assert that it's not empty" bit. Wondering if we should bite the bullet and pull in (or generate our own) nonempty collections. Probably not worth it for just this though. Sorry for the rambling.

I have needed a NonEmptyVec<T> many many times. I agree we should write one or use a good crate.

Would a newtype be better in this one case? There is also at least one widely-downloaded crate for this. I can't speak to its quality yet :)

jgallagher · 2026-09-10T18:44:00Z

 /// necessary for blueprint planning, and requires that the zone have a single
 /// IP.


"requires that the zone have a single IP" isn't right anymore

jgallagher · 2026-09-10T18:44:16Z

 /// necessary for blueprint planning, and requires that the zone have a single
 /// IP.


(same stale single IP comment)

jgallagher · 2026-09-10T18:45:08Z


        // NOTE: this is the *internal* DNS underlay address, held in
        // `second_service_ip` / `second_service_port`. External DNS's external
        // address comes from `external_ip` above.


- // address comes from `external_ip` above. + // addresses come from `external_ip_rows`.

jgallagher · 2026-09-10T18:47:45Z

        /// The service vNIC providing outbound connectivity using OPTE.
        pub nic: NetworkInterface,
-        pub external_ip: OmicronZoneExternalSnatIp,
+        /// The source NAT configuration (one address per IP family).


Does "one address per IP family" imply it always has both? But "up to one address per IP family" implies it could be no addrs at all. Yuck. Maybe delete the parenthetical entirely? 😅

Yeah, this always sucks to explain. It is: up to one address per family, and always at least one address.

bnaecker force-pushed the ben/multiple-eips-in-blueprint-zone-types branch 3 times, most recently from 36eb2df to 3a6bc8d Compare September 7, 2026 02:46

bnaecker force-pushed the ben/multiple-eips-in-blueprint-zone-types branch from 3a6bc8d to 9c66fa5 Compare September 7, 2026 14:05

bnaecker requested review from davepacheco and jgallagher September 9, 2026 03:44

jgallagher reviewed Sep 10, 2026

View reviewed changes

	dns_addresses: ExternalDnsAddrs::from_single(
	zone.dns_address.addr,
	),

		/// necessary for blueprint planning, and requires that the zone have a single
		/// IP.

Uh oh!

Conversation

bnaecker commented Sep 3, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants